[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692

carmenbianca · 2024-06-20T09:46:16Z

Needed for #691

Internal task: T12630

hr_timesheet_begin_end/models/account_analytic_line.py

huguesdk · 2024-06-24T07:45:20Z

hr_timesheet_begin_end/models/account_analytic_line.py

+        self._validate_start_before_stop()
+        self._validate_unit_amount_equal_to_time_diff()
+        self._validate_no_overlap()


this should be guarded with:

if not self.project_id: return

added some such guard using a filter.

should this add project_id to the constraint? if yes, should all the fields from _overlap_domain also be added to the constraint?

return [ ("id", "!=", self.id), ("employee_id", "=", self.employee_id.id), ("date", "=", self.date), ("time_start", "<", self.time_stop), ("time_stop", ">", self.time_start), ]

i think technically yes, but i am rather wary about constraints with a lot of fields.

huguesdk · 2024-06-24T07:57:19Z

hr_timesheet_begin_end/models/account_analytic_line.py

+        self._validate_no_overlap()
+
+    @api.model
+    def _hours_from_start_stop(self, time_start, time_stop):


what if one of time_start or time_stop is False?

I'm not sure this should happen because Floats default to 0 in Odoo.

huguesdk · 2024-06-24T08:01:17Z

hr_timesheet_begin_end/models/account_analytic_line.py

+    unit_amount = fields.Float(
+        compute="_compute_unit_amount",
+        store=True,
+        readonly=False,


shouldn’t this have an inverse function instead of readonly=False? the documentation doesn’t mention readonly=False (although the odoo code contains some of those) but states that to allow writing to a computed field, the inverse parameter must be used. this would also allow to correctly compute time_stop from time_start and unit_amount (but i don’t know whether this is actually useful).

not needed, i think. compute="_method", store=True, readonly=False is a pattern in odoo to compute whenever the requirements change, but to allow the user to override subsequently.

huguesdk · 2024-06-24T08:35:35Z

hr_timesheet_begin_end/models/account_analytic_line.py

+    def create(self, vals_list):
+        # This is a workaround for a bizarre situation: if a line is created
+        # with a time range but WITHOUT defining unit_amount, then you would
+        # expect unit_amount to be computed from the range. But for some reason,
+        # this never happens, and it is instead set to 0. Subsequently the
+        # constraint _validate_unit_amount_equal_to_time_diff kicks in and
+        # raises an exception.
+        #
+        # As a workaround, we add unit_amount with the correct value if it isn't
+        # in the vals_list. A better workaround might be to magically detect
+        # that unit_amount is not yet correctly computed in the constraint, and
+        # to skip the constraint if so.
+        self._add_unit_amount_to_vals_list(vals_list)
+        return super().create(vals_list)


i dug deeper into the problem and it comes from the fact that the analytic module defines unit_amount with default=0.0. when there is no default, stored computed fields are correctly computed at creation time, before constrains.

the only way to restore this behavior is to set default=None on the field. the compute function should then correctly set the field to its default value of 0.0 when needed.

i tried using a callable as the default value, but it must return a value, and any falsy value seems to be converted to 0, thus not resolving the problem.

i think it's fine to set default=None then.

hr_timesheet_begin_end/models/account_analytic_line.py

Timesheet lines are characterised by having a project. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

The separation of checks into separate methods is needed because I want to disable one check in another module. This makes the module more extensible. The unit_amount_from_start_stop method also makes the module more extensible. I have also moved the onchange to the top of the file, according to the OCA contribution guidelines. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

Instead of using onchange, which is less convenient in Odoo 16. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

This is a tricky one. _Technically_ there is nothing that makes hr_timesheet_begin_end incompatible with the other modules here. However, when the project_id on a timesheet line is changed, unit_amount is rerecomputed. This has some consequences. In the sale_timesheet tests, the project_id of a timesheet line is (sometimes?) recomputed. Subsequently, unit_amount is recomputed and set to 0 when it _shouldn't_ be. Under a normal flow this wouldn't be a problem; time_start and time_stop would be populated to correctly recompute unit_amount. But in the tests, they are not. One solution is to, in addition to not recomputing the unit_amount of non-timesheet lines, not recompute the unit_amount of timesheet lines whose time_start and time_stop are both set to 00:00. However, this means that resetting these values to 00:00 does not correctly set unit_amount back to 0, which seems erroneous to me. Marking this module as a rebel module seems like the best course of action to me, even though it would be preferable to test this in conjunction with the other modules. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

carmenbianca · 2024-06-28T12:55:48Z

I added hr_timesheet_begin_end as rebel module. See the commit message.

huguesdk

just a comment about a (too long) comment. otherwise lgtm! thanks @carmenbianca!

huguesdk · 2024-09-02T09:18:22Z

hr_timesheet_begin_end/models/account_analytic_line.py

+        # This default is a workaround for a bizarre situation: if a line is
+        # created with a time range but WITHOUT defining unit_amount, then you
+        # would expect unit_amount to be computed from the range. But this never
+        # happens, and it is instead set to default value 0. Subsequently the
+        # constraint _validate_unit_amount_equal_to_time_diff kicks in and
+        # raises an exception.
+        #
+        # By setting the default to None, the computation is correctly
+        # triggered. If nothing is computed, None falls back to 0.


i think that the comment can be simpler. something like:

Suggested change

# This default is a workaround for a bizarre situation: if a line is

# created with a time range but WITHOUT defining unit_amount, then you

# would expect unit_amount to be computed from the range. But this never

# happens, and it is instead set to default value 0. Subsequently the

# constraint _validate_unit_amount_equal_to_time_diff kicks in and

# raises an exception.

#

# By setting the default to None, the computation is correctly

# triggered. If nothing is computed, None falls back to 0.

# removing the default of 0.0 to ensure it is computed when not

# provided. if not computed (because project_id is False), None is

# automatically converted to 0.

carmenbianca mentioned this pull request Jun 20, 2024

[16.0][ADD] hr_timesheet_begin_end_no_end #691

Draft

carmenbianca force-pushed the 16.0-refactor-hr_timesheet_begin_end branch 2 times, most recently from 299a3b8 to ae7ea83 Compare June 20, 2024 13:27

carmenbianca commented Jun 20, 2024

View reviewed changes

hr_timesheet_begin_end/models/account_analytic_line.py Outdated Show resolved Hide resolved

carmenbianca commented Jun 20, 2024

View reviewed changes

hr_timesheet_begin_end/models/account_analytic_line.py Outdated Show resolved Hide resolved

carmenbianca force-pushed the 16.0-refactor-hr_timesheet_begin_end branch from ae7ea83 to 0423bc6 Compare June 20, 2024 15:08

carmenbianca mentioned this pull request Jun 20, 2024

[16.0][ADD] hr_timesheet_overtime_rate_begin_end coopiteasy/cie-timesheet#33

Open

3 tasks

carmenbianca force-pushed the 16.0-refactor-hr_timesheet_begin_end branch from 0423bc6 to 1212f36 Compare June 21, 2024 09:33

carmenbianca mentioned this pull request Jun 21, 2024

[12.0][REF] hr_timesheet_activity_begin_end: Make more extensible #693

Open

huguesdk suggested changes Jun 24, 2024

View reviewed changes

carmenbianca added 2 commits June 28, 2024 11:41

[FIX] hr_timesheet_begin_end: Test uses timesheet lines now

764a1c0

Timesheet lines are characterised by having a project. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

carmenbianca force-pushed the 16.0-refactor-hr_timesheet_begin_end branch from 1212f36 to 23acc70 Compare June 28, 2024 09:41

[REF] hr_timesheet_begin_end: Refactor unit_amount to be a compute field

55c67c2

Instead of using onchange, which is less convenient in Odoo 16. Signed-off-by: Carmen Bianca BAKKER <[email protected]>

carmenbianca force-pushed the 16.0-refactor-hr_timesheet_begin_end branch from 23acc70 to 55c67c2 Compare June 28, 2024 11:53

carmenbianca requested a review from huguesdk June 28, 2024 12:55

huguesdk approved these changes Sep 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692

[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692

carmenbianca commented Jun 20, 2024

huguesdk Jun 24, 2024

carmenbianca Jun 28, 2024

huguesdk Jun 24, 2024

carmenbianca Jun 28, 2024

huguesdk Jun 24, 2024

carmenbianca Jun 28, 2024

huguesdk Jun 24, 2024

carmenbianca Jun 28, 2024

carmenbianca commented Jun 28, 2024

huguesdk left a comment

huguesdk Sep 2, 2024

[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692

Are you sure you want to change the base?

[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692

Conversation

carmenbianca commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmenbianca commented Jun 28, 2024

huguesdk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment